fix(ui): throttle self-serve SSO domain verification retries - #9275
fix(ui): throttle self-serve SSO domain verification retries#9275NicolasLopes7 wants to merge 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 77081cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
@clerk/sharedCurrent version: 4.27.0 Subpath
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConfigureSSO adds ownership-verification retries with a five-minute cooldown. Expired and unverified TXT-record states use a shared retry control. The control displays throttled-state messaging. Tests cover retry visibility, cooldown behavior, countdown messaging, failed requests, and timer restoration. Localization resources and a patch changeset document the behavior. Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
| const retryTimerRef = useRef<number | undefined>(undefined); | ||
| const isVerificationRetryThrottledRef = useRef(false); | ||
|
|
There was a problem hiding this comment.
what's the thinking behind the use of useRef to store timeouts? I feel like useState might be more appropriate but I'd like to understand more
There was a problem hiding this comment.
Claude tripped here, just addressed
Drop the ref that mirrored the throttle flag: the click handler closes over current state, and the button is already disabled while throttled. Track the cooldown as a deadline instead of a boolean, start it only after prepareOwnershipVerification resolves, and show the remaining time on the disabled retry button. Move the retry action inline with the TXT record value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx`:
- Around line 609-616: Add a dedicated element descriptor to
elementDescriptors.ts, then import and pass it via the elementDescriptor prop on
the retry Button in OrganizationDomainsStep. Keep the descriptor specific to
this retry control so it is available for Appearance customization.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d9cf024-57d0-48b8-8c5c-faa2fe1f4f19
📒 Files selected for processing (5)
.changeset/bright-trees-retry.mdpackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/bright-trees-retry.md
| <Button | ||
| variant='bordered' | ||
| colorScheme='secondary' | ||
| size='xs' | ||
| isLoading={isVerifying} | ||
| isDisabled={isThrottled} | ||
| onClick={onClick} | ||
| sx={[t => ({ flexShrink: 0, gap: t.space.$1x5 }), sx]} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an element descriptor for the retry button.
Line 609 creates a new themable Button without elementDescriptor. Add a dedicated descriptor in packages/ui/src/customizables/elementDescriptors.ts and pass it to this button. This keeps the new retry control customizable through Appearance configuration.
As per coding guidelines, “Use element descriptors for all themable elements by applying elementDescriptor prop to components and importing descriptors from packages/ui/src/customizables/elementDescriptors.ts.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx`
around lines 609 - 616, Add a dedicated element descriptor to
elementDescriptors.ts, then import and pass it via the elementDescriptor prop on
the retry Button in OrganizationDomainsStep. Keep the descriptor specific to
this retry control so it is available for Appearance customization.
Source: Coding guidelines
Build the retry button once in DomainCard and pass it down as a node, mirroring the existing removeButton pattern, instead of drilling three props through ExpiredNotice and TxtRecord. Drop the live countdown so the cooldown needs a single setTimeout rather than a per-second interval plus its derived-state mirror and formatter.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx (1)
86-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not throttle resolved domain-preparation failures.
The bulk API reports a failed domain in its resolved
errorsarray. This helper returnstruefor every fulfilled request.DomainCardthen disables retries for five minutes after a failed preparation.Return
falsewhen the response contains an error for this one-domain request. Add coverage for a fulfilled response witherrors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx` around lines 86 - 95, Update handlePrepareOwnershipVerification to inspect the fulfilled prepareOwnershipVerification response and return false when its errors array contains an error for the requested domain; return true only when preparation succeeds without errors. Preserve existing catch handling and add coverage for a fulfilled response containing errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx`:
- Around line 86-95: Update handlePrepareOwnershipVerification to inspect the
fulfilled prepareOwnershipVerification response and return false when its errors
array contains an error for the requested domain; return true only when
preparation succeeds without errors. Preserve existing catch handling and add
coverage for a fulfilled response containing errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cbe87816-f26e-4ad5-b1c6-1e02e99b0a5d
📒 Files selected for processing (5)
.changeset/bright-trees-retry.mdpackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/ConfigureSSO/__tests__/ConfigureSSO.test.tsxpackages/ui/src/components/ConfigureSSO/steps/OrganizationDomainsStep.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/localizations/src/en-US.ts
- .changeset/bright-trees-retry.md
- packages/ui/src/components/ConfigureSSO/tests/ConfigureSSO.test.tsx
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Adds the verifyAgainButtonTooltip__throttled placeholder to the other locales, as the generate step requires.
The button calls prepare, which rotates the TXT challenge, while the UI already polls attempt on its own. "Verify again" read as a manual check, so clicking it while waiting on DNS silently invalidated the record the admin had already published.
2 participants
Summary
Testing